Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add referenced class objects to JavaClass #518

Merged
merged 5 commits into from
Jan 31, 2021

Conversation

codecholeric
Copy link
Collaborator

@codecholeric codecholeric commented Jan 27, 2021

So far ArchUnit has not been able to detect the pure usage of class objects as dependencies. E.g. the following example would not have detected a dependency on Evil, since besides the reference to the class object no further dependency on Evil (like a field access or method call) has occurred.

class Example {
  final Map<Class<?>, Object> association = Map.of(Evil.class, anything);
}

With this PR JavaClass now knows its referencedClassObjects, including the respective line number. Furthermore class objects are now parts of the dependencies{From/To}Self of a JavaClass.

Resolves: #309
Issue: #446
Resolves: #474
Resolves: #515

@codecholeric codecholeric force-pushed the add-loaded-class-constants-to-JavaClass branch from 8e4841a to 6fb8b40 Compare January 27, 2021 21:56
While `toString()` is not really intended as API we should always make sure there is some reasonable string output for debugging purposes.

Signed-off-by: Peter Gafert <[email protected]>
We should unify this and let all domain objects that have a designated source code location implement `HasSourceCodeLocation`.

Signed-off-by: Peter Gafert <[email protected]>
A method taking a `String` as parameter should clearly state what format the string has. A `String` is not an "ASM type".

Signed-off-by: Peter Gafert <[email protected]>
@codecholeric codecholeric force-pushed the add-loaded-class-constants-to-JavaClass branch from 6fb8b40 to 5c794ae Compare January 30, 2021 18:13
@codecholeric codecholeric changed the title Add loaded class constants to JavaClass Add referenced class objects to JavaClass Jan 30, 2021
@codecholeric codecholeric force-pushed the add-loaded-class-constants-to-JavaClass branch from 5c794ae to 37f103a Compare January 30, 2021 18:17
@codecholeric codecholeric requested a review from kaebiscs January 30, 2021 18:19
@codecholeric
Copy link
Collaborator Author

Thanks a lot for the review @kaebiscs ! I agree that ReferencedClassObject is a way better name from a user's perspective 😄 I've tried to adjust all the places to this new terminology. Can you check again real quick that everything looks good to you?

Copy link

@kaebiscs kaebiscs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review changes look good to me.

To detect usages of class objects we need to look which classes are referenced in the constant pool. So far the following usage of `SomeClass` would not have been detected by ArchUnit:

```
class Example {
  private Map<Class<?>, Value> map = Map.of(SomeClass.class, someValue);
}
```

In other words, pure usages of `Foo.class` could not be detected, because there was no "access" to `Foo` in the bytecode. However, for each such occurrence the class would actually be present in the constant pool of the referring class. While ASM does not allow direct access to the constant pool, it does allow us to hook into `ldc` instructions, i.e. load constant instructions in the bytecode, that will in turn reference the respective class. The way to detect this is principally an `instanceof` check for `org.objectweb.asm.Type` in `void visitLdcInsn(Object value)`. As far as I could see, any reference of another class as a constant would cause this method to be called with the respective ASM `Type` object.

It would actually be possible to import a lot more constants here. I have looked a little into adding all supported constant types, so it would e.g. be possible to have assertions on `String` values, but then decided to let it go for now. Strings would still be simple, but as soon as `Integer` comes into play (which could also be imported), there are a lot of internal optimizations by the JVM (e.g. `iconst_1`, ...), which makes it hard to do this in a consistent way. I think the most valuable feature by far is to detect constant loads of classes (since those cause dependencies), so I decided to keep it simple for now.

Signed-off-by: Peter Gafert <[email protected]>
@codecholeric codecholeric force-pushed the add-loaded-class-constants-to-JavaClass branch from 37f103a to 806b04d Compare January 30, 2021 20:01
@codecholeric codecholeric force-pushed the add-loaded-class-constants-to-JavaClass branch from 806b04d to 1263ae5 Compare January 31, 2021 12:47
@codecholeric codecholeric merged commit 8777583 into master Jan 31, 2021
@codecholeric codecholeric deleted the add-loaded-class-constants-to-JavaClass branch January 31, 2021 13:04
@codecholeric codecholeric mentioned this pull request Feb 1, 2021
codecholeric added a commit that referenced this pull request Feb 21, 2021
So far ArchUnit has not been able to detect the pure usage of class objects as dependencies. E.g. the following example would not have detected a dependency on `Evil`, since besides the reference to the class object no further dependency on `Evil` (like a field access or method call) has occurred.

```
class Example {
  final Map<Class<?>, Object> association = Map.of(Evil.class, anything);
}
```

With this PR `JavaClass` now knows its `referencedClassObjects`, including the respective line number. Furthermore class objects are now parts of the `dependencies{From/To}Self` of a `JavaClass`.

Resolves: #309 
Issue: #446 
Resolves: #474 
Resolves: #515
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependency do not support constant Cycles not detected Import constant pool of classes
2 participants